Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix err 500 on listing /api/v1/namespaces with browsable api enabled #1915

Merged
merged 10 commits into from
Oct 23, 2023
Merged

Fix err 500 on listing /api/v1/namespaces with browsable api enabled #1915

merged 10 commits into from
Oct 23, 2023

Conversation

jerabekjiri
Copy link
Contributor

@jerabekjiri jerabekjiri commented Oct 3, 2023 •
edited
Loading

Issue: AAH-2733

This feels like a hacky fix. I'm not sure why django says the method is POST on visiting the endpoint if browsable api is enabled.
For some reason, django browsable api form make POST (create) request which triggers access policy (is_namespace_owner) just on visiting endpoint.

With browsable api enabled: visiting endpoint "request.method" is "POST", "action" is "create", even though request.META["REQUEST_METHOD"] is "GET".

(if ?format=json, endpoint works correctly even basic user is logged in)
Still investigating.

@github-actions github-actions bot added backport-4.2 This PR should be backported to stable-4.2 (1.2) backport-4.4 This PR should be backported to stable-4.4 (2.1) backport-4.5 This PR should be backported to stable-4.5 (2.2) backport-4.6 This PR should be backported to stable-4.6 (2.3) backport-4.7 This PR should be backported to stable-4.7 (2.4) backport-4.8 This PR should be backported to stable-4.8 (2.4) labels Oct 3, 2023
@jerabekjiri jerabekjiri removed backport-4.2 This PR should be backported to stable-4.2 (1.2) backport-4.4 This PR should be backported to stable-4.4 (2.1) backport-4.5 This PR should be backported to stable-4.5 (2.2) backport-4.6 This PR should be backported to stable-4.6 (2.3) backport-4.7 This PR should be backported to stable-4.7 (2.4) backport-4.8 This PR should be backported to stable-4.8 (2.4) labels Oct 4, 2023
@jerabekjiri
Copy link
Contributor Author

@drodowic Is it okay to use the PULP_CONTENT_ORIGIN variable to enable the CustomBrowsableAPIRenderer renderer class only for galaxy.ansible.com (and also for dev and stage)? I don't think I have any other way to test this.

@drodowic
Copy link
Collaborator

@drodowic Is it okay to use the PULP_CONTENT_ORIGIN variable to enable the CustomBrowsableAPIRenderer renderer class only for galaxy.ansible.com (and also for dev and stage)? I don't think I have any other way to test this.

PULP_CONTENT_ORIGIN is explicitly set in each environment for galaxy.ansible.com so yes I think it's unique enough to use in this case.

rochacbruno
rochacbruno requested changes Oct 13, 2023
View reviewed changes
docker/etc/settings.py Outdated Show resolved Hide resolved
galaxy_ng/app/dynaconf_hooks.py Outdated Show resolved Hide resolved
@jerabekjiri jerabekjiri force-pushed the fix/error-500-v1-namespaces branch from eae2013 to 3bc42ee Compare October 16, 2023 11:44
@jctanner
Copy link
Collaborator

Does this problem exist on the devstack in a way that we could have reproduced it?

@jerabekjiri
Copy link
Contributor Author

@jctanner I can probably add test with ?format=api to test the behavior.

@jerabekjiri jerabekjiri force-pushed the fix/error-500-v1-namespaces branch from 3bc42ee to 72d040c Compare October 18, 2023 22:26
@jerabekjiri
test with basic_user
29e720a 
Issue: AAH-2733
@jerabekjiri jerabekjiri merged commit 9823b29 into ansible:master Oct 23, 2023
19 checks passed
@jerabekjiri jerabekjiri mentioned this pull request Oct 23, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@drodowic drodowic drodowic approved these changes

@jctanner jctanner jctanner approved these changes

@rochacbruno rochacbruno rochacbruno approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jerabekjiri @drodowic @jctanner @rochacbruno